Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take glTF up axis into account during upgrade #166

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jan 8, 2025

Addresses #165

What this currently does:

  • It checks for the presence of the tileset.asset.gltfUpAxis
  • If it is found, then it is removed from the tileset JSON, and passed on the the tile content upgrades
  • Until now, this is only used for the conversion of B3DM with glTF 1.0 into glTF 2.0 (when upgrading to 1.1)

It could be desirable to also use this for the upgrade to 1.0: The gltfUpAxis is a "legacy" feature that should not be part of a tileset that has been upgraded to 1.0. And this might be doable with reasonable effort.

What I'm more scared of is that the gltfUpAxis also (likely) will affect the I3DM upgrade. The coordinate system conversions for I3DM had been a hassle. The coordinate system conversions for the gltfUpAxis are a hassle. Combining this will be hassle², with the caveat that this involves generating that "legacy" data in the first place...

The current test data (which is not yet checked in, and might still change) is here:

upAxisHandling 2025-01-08.zip

It focusses on the B3DM case (with and without an RTC_CENTER in the B3DM), and makes sure that the visual appearance of the output is still the same as the input in CesiumJS (Sandcastle and output are included)

Cesium Axes 2025-01-08

This is supposed to eventually become a test data set for the specs, using the "golden" comparison approach.

@javagl javagl marked this pull request as draft January 9, 2025 11:34
@javagl
Copy link
Contributor Author

javagl commented Jan 14, 2025

The last commits consisted of cleanups, adding the same functionality for I3DM, and creating/adding the corresponding test data as specs.

The specs use the "golden" approach: The upgradeTileset/upAxisHandling directory contains the input directory, with the various test cases, and the golden directory with the expected output. When the TilesetUpgraderAxesSpec is run, then it will write the results into an output directory and compare them to the golden one.

The directory also contains a short README and a Sandcastle for comparing the input and output.

The case of gltfUpAxis+I3DM indeed brought up some issue, but ... one can argue whether this is something that should be handled: When the glTF 1.0 in an I3DM uses Z- or X-up, and this is not declared via the gltfUpAxis in the tileset JSON, then the upgrade does not generate a proper (unmodified) GLB. I'd have to spend much more time (with creating test data, and probably pen+paper) to figure out whether this can sensibly be addressed, and how: The conversion of the I3DM into EXT_mesh_gpu_instancing inevietably has to make assumptions about the meaning of the translations in the I3DM, and how they should be transferred into the glTF extension.

The following shows the Sandcastle for comparing the inputs and outputs, pausing for a moment in this case:

Cesium Up Axis Handling Sandcastle 0001

I think that this can be justified: The upgrade cannot magically "know" how the data was supposed to be interpreted. And when the gltfUpAxis is specified, then the upgrade works as expected.


Beyond that, there may still be a million cases that don't work. Maybe some tileset with gltfUpAxis=Z that refers to external tilesets with gltfUpAxis=X? Or obscure combinations of the CESIUM_RTC extension and an RTC_CENTER in the B3DM? ...

What I did test, though, was the current state with a user-provided real-world data set that used gltfUpAxis=Z, and the CESIUM_RTC extension, and I made sure that the result is the expected one. So even when there may be many cases where ~"something that may appear in legacy data" does not work, at least one real-world configuration of legacy data can be properly upgraded now.

@javagl javagl marked this pull request as ready for review January 14, 2025 22:48
@lilleyse lilleyse merged commit dc1ec1a into main Jan 16, 2025
2 checks passed
@lilleyse lilleyse deleted the upgrade-gltf-up-axis branch January 16, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants